modify nakamoto block header to use `Vec<MessageSignature>` #4781

hstove · 2024-05-13T03:04:46Z

Closes Nakamoto Signer[3.0] - Update Block Header to use a vec of Signatures #4776

This builds on top of #4778, to try and reduce duplicate work and conflicts.

The verify_signer_signatures function has a few rules:

The Vec<Signature> can be smaller than Vec<Signer>, but it's still order dependent.
- What this means is that if you have signers A,B,C, you can have signatures sA,sC, but not sC,sA.
This depends on the signers being unique by public key (which I think is true, but if not it requires refactoring)
I'm summing the total weight of the RewardSet and using 70% as the threshold
No duplicates, no signatures from non-signers, etc

Tracking the work included in this PR:

Updates the signer_signature field in NakamotoBlockHeader to be Vec<MessageSignature>
Adds a new verify_signer_signatures method to NakamotoBlockHeader with tests
Verify the block header in accept_block
Update TestSigners and TestSigningChannel to sign the block using the new format
Update blind_signer related code for Nakamoto integrations

What I think (?) is out of scope:

Some kind of epoch gating around when/if it verify signatures (maybe out of scope for this PR)
Miner updates to gather signatures (Nakamoto Signer[3.0] - The sign_coordinator must be updated to send a block proposal message rather than initiate a signing round #4775)
- Work is started on this, but right now it only uses TestSigningChannel
Signer updates to broadcast signature (Nakamoto Signer[3.0] Create a new process_event path for v0 signer to handle v0 message types #4785)
Verify signatures in the block downloader and other parts of the p2p stack (Update Nakamoto downloader to utilize signatures vec instead of aggregate public key #4793)

stackslib/src/chainstate/nakamoto/mod.rs

jcnelson · 2024-05-15T16:26:30Z

Hi @hstove, I suspect that you have discovered that in order to ship this PR, a lot of unit test infrastructure needs to be changed as well (see #4796). I wonder if it makes sense to create a feature branch in the Stacks blockchain for switching over from WSTS to multisig?

hstove · 2024-05-15T20:15:58Z

@jcnelson I think that's possibly the right idea, but I'm finding it to be actually pretty reasonable to swap the testing infra without really messing it up for when we go back to threshold sigs. There are a few places (ie TestSigners, SignCoordinator) where I'm keeping essentially two implementations, and only one of them is actively used.

I have a number of TODO comments in this PR, which are mainly related to where you'd need to do epoch-gated behavior. I'd love to see what you think about how to proceed given my first attempt at this.

hstove · 2024-05-15T20:18:34Z

@jferrant @kantai @obycode tagging you in as I think this PR is in a good place to get a review. I'll want to see a full CI run to see exactly what's still TBD, but a lot of the initial pieces are in place.

I've also rebased this on top of #4788, as that includes some type changes that are utilized.

kantai

This LGTM -- just a few comments. Exciting stuff!

stackslib/src/chainstate/nakamoto/mod.rs

testnet/stacks-node/src/nakamoto_node/sign_coordinator.rs

stackslib/src/chainstate/nakamoto/mod.rs

stackslib/src/chainstate/nakamoto/test_signers.rs

jcnelson

Overall LGTM. Just a couple merge conflicts and a couple places where an error shouldn't be fatal.

hstove · 2024-06-03T18:59:39Z

@jcnelson I've refactored run_miner to return a Result, along with removing any panics. Can you give 55567d7 a look and see if that's on track with your suggestions?

kantai · 2024-06-04T18:26:10Z

Approving.

I pushed 7c8df31. The miner thread joining logic needs to allow the prior mining thread to error (the prior miner will exit with an error if it detects the burnchain change). I am pretty sure that this was what was causing the CI failures (it caused those tests to fail locally for me).

hstove · 2024-06-04T18:26:56Z

Nice catch, thank you @kantai!

blockstack-devops · 2024-10-31T00:20:49Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

hstove requested a review from kantai May 13, 2024 03:04

hstove force-pushed the feat/header-signer-signatures branch from 6abb574 to 8c92e2d Compare May 13, 2024 22:33

hstove commented May 13, 2024

View reviewed changes

stackslib/src/chainstate/nakamoto/mod.rs Outdated Show resolved Hide resolved

hstove force-pushed the feat/header-signer-signatures branch from 8c92e2d to fe3ca6f Compare May 13, 2024 23:00

jcnelson mentioned this pull request May 15, 2024

Update unit test signer to use signatures instead of aggregate public key #4796

Closed

hstove changed the base branch from chore/signer-traits to feat/add-v0-signer May 15, 2024 20:10

hstove force-pushed the feat/header-signer-signatures branch from fe3ca6f to d14e5b4 Compare May 15, 2024 20:11

hstove requested review from jcnelson, obycode and jferrant May 15, 2024 20:16

saralab marked this pull request as ready for review May 16, 2024 13:51

kantai reviewed May 16, 2024

View reviewed changes